cloud_storage: include revision_id in spillover manifest cache key by nvartolomei · Pull Request #29068 · redpanda-data/redpanda

nvartolomei · 2025-12-19T12:43:09Z

Fix cache collision where spillover manifests from a deleted topic could
be applied to a newly created topic with the same ntp. The cache key now
includes revision_id to distinguish between different topic incarnations.

Adds test that reproduces the issue by creating a topic, applying
spillover, deleting the topic, and recreating it with the same name.

Backports Required

Release Notes

Bug Fixes

Fix a cache collision in in-memory spillover manifest cache which could lead to new topic reading data from the old topic with the same name for a short period of time.

vbotbuildovich · 2025-12-19T15:50:11Z

CI test results

test results on build#78196

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason	test_history
PartitionBalancerTest	test_rack_awareness	null	integration	https://buildkite.com/redpanda/redpanda/builds/78196#019b36dc-9f81-4f4a-9ffc-8cac1d602bb4	FLAKY	10/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0000, p0=1.0000, reject_threshold=0.0100. adj_baseline=0.1000, p1=0.3487, trust_threshold=0.5000)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=PartitionBalancerTest&test_method=test_rack_awareness
ScalingUpTest	test_fast_node_addition	null	integration	https://buildkite.com/redpanda/redpanda/builds/78196#019b36dc-9f86-4ab3-8951-e2b31e7e5f2d	FLAKY	10/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.0318, p0=1.0000, reject_threshold=0.0100. adj_baseline=0.1000, p1=0.3487, trust_threshold=0.5000)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=ScalingUpTest&test_method=test_fast_node_addition

test results on build#78253

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason	test_history
MountUnmountIcebergTest	test_simple_remount	{"cloud_storage_type": 1}	integration	https://buildkite.com/redpanda/redpanda/builds/78253#019b3aff-63e9-4047-9d2c-bca9f2ecd3e4	FLAKY	9/11	Test PASSES after retries.No significant increase in flaky rate(baseline=0.1620, p0=0.8291, reject_threshold=0.0100. adj_baseline=0.4114, p1=0.0399, trust_threshold=0.5000)	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=MountUnmountIcebergTest&test_method=test_simple_remount

oleiman

lgtm. awesome find.

src/v/cloud_storage/tests/cloud_storage_e2e_test.cc

oleiman · 2025-12-19T17:24:23Z

src/v/cloud_storage/tests/cloud_storage_e2e_test.cc

+// Test a scenario where after a topic recreation the spillover manifests from
+// the previous incarnation could be applied to the new topic, causing errors or
+// reading wrong data.


for posterity, this test would consistently fail before your fix?

Yes. Every single time. The test was written before the fix.

I need to an assert though to make sure that spillover did actually happen to future proof it.

Every single time

😞

assert ... to make sure that spillover did actually happen

good idea 👍

Fix cache collision where spillover manifests from a deleted topic could be applied to a newly created topic with the same ntp. The cache key now includes revision_id to distinguish between different topic incarnations. Adds test that reproduces the issue by creating a topic, applying spillover, deleting the topic, and recreating it with the same name.

Copilot

Pull request overview

This PR fixes a cache collision bug in the spillover manifest cache where manifests from a deleted topic could be incorrectly applied to a newly created topic with the same ntp. The fix adds revision_id to the cache key to distinguish between different topic incarnations.

Key Changes:

Updated manifest cache key from tuple<ntp, offset> to tuple<ntp, revision_id, offset>
Added test reproducing the collision scenario (create topic, apply spillover, delete, recreate)
Modified test utilities to support sequential record generation across topic recreations

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/v/cloud_storage/materialized_manifest_cache.h	Updated cache key type definition to include revision_id
src/v/cloud_storage/materialized_manifest_cache.cc	Updated cache key construction and formatting to include revision_id
src/v/cloud_storage/async_manifest_materializer.cc	Updated all cache lookups to include revision_id in the key
src/v/cloud_storage/tests/materialized_manifest_cache_test.cc	Updated test helper functions to construct keys with revision_id
src/v/cloud_storage/tests/cloud_storage_e2e_test.cc	Added regression test for spillover cache collision bug
src/v/cloud_storage/tests/produce_utils.h	Enhanced generator to support continuing sequence across invocations

Copilot · 2025-12-20T08:00:04Z

src/v/cloud_storage/tests/produce_utils.h

+    remote_segment_generator& start_ix(size_t ix) {
+        _current_ix = ix;
+        return *this;
+    }


The new start_ix method lacks documentation explaining its purpose and when it should be used. Add a comment describing that this method sets the starting index for record generation, useful for continuing sequences across multiple invocations or topic incarnations.

Copilot · 2025-12-20T08:00:05Z

src/v/cloud_storage/tests/cloud_storage_e2e_test.cc

+        vlog(e2e_test_log.info, "Running iteration {}", iteration);
+
+        add_topic({model::kafka_namespace, topic_name}, 1, props).get();
+        wait_for_leader(ntp).get();


The comment 'Seeding partition data' is correct, but consider making it more descriptive for this specific iteration context, such as 'Seeding partition data for iteration N' to improve test output clarity.

Suggested change

wait_for_leader(ntp).get();

SCOPED_TRACE(

fmt::format("Seeding partition data for iteration {}", iteration));

Copilot · 2025-12-20T08:00:05Z

src/v/cloud_storage/tests/cloud_storage_e2e_test.cc

+        // Consume all data.
+        tests::kafka_consume_transport consumer(make_kafka_client().get());
+        auto deferred_c_close = ss::defer(
+          [&consumer] { consumer.stop().get(); });


The test verifies only the first record's key matches the expected iteration value. Consider also checking the last record or a sample of records to ensure all consumed data is from the correct iteration and not mixed with data from the previous topic incarnation.

vbotbuildovich · 2025-12-21T06:23:42Z

/backport v25.3.x

vbotbuildovich · 2025-12-21T06:23:43Z

/backport v25.2.x

vbotbuildovich · 2025-12-21T06:23:44Z

/backport v25.1.x

vbotbuildovich · 2025-12-21T06:24:46Z

Failed to create a backport PR to v25.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-29068-v25.2.x-603 remotes/upstream/v25.2.x
git cherry-pick -x dfceec48d9 379d42743c

Workflow run logs.

vbotbuildovich · 2025-12-21T06:24:58Z

Failed to create a backport PR to v25.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-29068-v25.1.x-83 remotes/upstream/v25.1.x
git cherry-pick -x dfceec48d9 379d42743c

Workflow run logs.

cloud_storage/tests: allow seeding with base index

dfceec4

nvartolomei requested review from Lazin and Copilot and removed request for Copilot December 19, 2025 12:43

github-actions bot added the area/redpanda label Dec 19, 2025

nvartolomei requested review from andrwng and oleiman December 19, 2025 12:43

oleiman previously approved these changes Dec 19, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings December 20, 2025 07:59

nvartolomei dismissed oleiman’s stale review via 379d427 December 20, 2025 07:59

nvartolomei force-pushed the nv/spillover-cache-collision branch from 9c7f258 to 379d427 Compare December 20, 2025 07:59

Copilot AI reviewed Dec 20, 2025

View reviewed changes

rockwotj approved these changes Dec 21, 2025

View reviewed changes

nvartolomei merged commit 9a7a69b into redpanda-data:dev Dec 21, 2025
19 checks passed

vbotbuildovich mentioned this pull request Dec 21, 2025

[v25.2.x] cloud_storage: include revision_id in spillover manifest cache key #29085

Closed

This was referenced Dec 21, 2025

[v25.3.x] cloud_storage: include revision_id in spillover manifest cache key #29086

Merged

[v25.1.x] cloud_storage: include revision_id in spillover manifest cache key #29087

Closed

This was referenced Dec 22, 2025

[v25.2.x] cloud_storage: include revision_id in spillover manifest cache key #29089

Merged

[v25.1.x] cloud_storage: include revision_id in spillover manifest cache key #29090

Merged

	wait_for_leader(ntp).get();
	SCOPED_TRACE(
	fmt::format("Seeding partition data for iteration {}", iteration));

Conversation

nvartolomei commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backports Required

Release Notes

Bug Fixes

Uh oh!

vbotbuildovich commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI test results

Uh oh!

oleiman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleiman Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

nvartolomei Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

oleiman Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbotbuildovich commented Dec 21, 2025

Uh oh!

vbotbuildovich commented Dec 21, 2025

Uh oh!

vbotbuildovich commented Dec 21, 2025

Uh oh!

vbotbuildovich commented Dec 21, 2025

Uh oh!

vbotbuildovich commented Dec 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nvartolomei commented Dec 19, 2025 •

edited

Loading

vbotbuildovich commented Dec 19, 2025 •

edited

Loading